Unify first parameter name to path_or_io across methods#358
Merged
Conversation
alstrocrack
changed the title
filename_or_io across methods
Member
|
Hmm. Let's use |
Contributor
Author
|
Thank you for your suggestion. |
Member
|
Could you also update the PR title and description? |
There was a problem hiding this comment.
Pull request overview
This PR aims to standardize the first parameter name across CSV class methods to better communicate that callers may pass either a file path or an IO-like object, and to align the corresponding RDoc wording.
Changes:
- Renames the first parameter for
CSV.foreach,CSV.read,CSV.readlines, andCSV.tableto a unified name. - Renames
CSV.open’s first parameter and the related private helper parameter to match the unified name. - Updates related RDoc text to reference the unified parameter name.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1389
to
+1391
| def foreach(path_or_io, mode="r", **options, &block) | ||
| return to_enum(__method__, path_or_io, mode, **options) unless block_given? | ||
| open(path_or_io, mode, **options) do |csv| |
Comment on lines
+1647
to
1652
| def open(path_or_io, mode="r", **options) | ||
| # wrap a File opened with the remaining +args+ with no newline | ||
| # decorator | ||
| file_opts = {} | ||
| may_enable_bom_detection_automatically(filename_or_io, | ||
| may_enable_bom_detection_automatically(path_or_io, | ||
| mode, |
Comment on lines
+1922
to
+1931
| def read(path_or_io, **options) | ||
| open(path_or_io, **options) { |csv| csv.read } | ||
| end | ||
|
|
||
| # :call-seq: | ||
| # CSV.readlines(source, **options) | ||
| # | ||
| # Alias for CSV.read. | ||
| def readlines(path, **options) | ||
| read(path, **options) | ||
| def readlines(path_or_io, **options) | ||
| read(path_or_io, **options) |
Comment on lines
+1949
to
+1956
| def table(path_or_io, **options) | ||
| default_options = { | ||
| headers: true, | ||
| converters: :numeric, | ||
| header_converters: :symbol, | ||
| } | ||
| options = default_options.merge(options) | ||
| read(path, **options) | ||
| read(path_or_io, **options) |
alstrocrack
changed the title
filename_or_io across methodspath_or_io across methods
Comment on lines
+1389
to
+1391
| def foreach(path_or_io, mode="r", **options, &block) | ||
| return to_enum(__method__, path_or_io, mode, **options) unless block_given? | ||
| open(path_or_io, mode, **options) do |csv| |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
lib/csv.rb:1940
- The
:call-seq:fortablestill usessourcewhile the method signature is nowtable(path_or_io, **options). Since the PR’s goal is to unify the first parameter name, the:call-seq:should also be updated topath_or_ioto keep docs consistent.
# :call-seq:
# CSV.table(source, **options)
#
# Calls CSV.read with +source+, +options+, and certain default options:
# - +headers+: +true+
# - +converters+: +:numeric+
# - +header_converters+: +:symbol+
Comment on lines
1584
to
1586
| # * This method optionally accepts an additional <tt>:encoding</tt> option | ||
| # that you can use to specify the Encoding of the data read from +path+ or +io+. | ||
| # that you can use to specify the Encoding of the data read from +path_or_io+. | ||
| # You must provide this unless your data is in the encoding |
Contributor
Author
There was a problem hiding this comment.
I also think this should be fixed, but it shouldn’t be done in this PR.
Comment on lines
1921
to
+1923
| # CSV.read(path, headers: true) # => #<CSV::Table mode:col_or_row row_count:4> | ||
| def read(path, **options) | ||
| open(path, **options) { |csv| csv.read } | ||
| def read(path_or_io, **options) | ||
| open(path_or_io, **options) { |csv| csv.read } |
Comment on lines
+1927
to
+1931
| # CSV.readlines(source, **options) | ||
| # | ||
| # Alias for CSV.read. | ||
| def readlines(path, **options) | ||
| read(path, **options) | ||
| def readlines(path_or_io, **options) | ||
| read(path_or_io, **options) |
Contributor
Author
|
I fixed this PR’s title, description, and all the suggestions from GitHub Copilot. Could you please check it again? |
Member
|
Thanks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Previously, the first parameter name was inconsistent across methods and their RDoc comments, with names such as
path,source, andfilename_or_ioused interchangeably despite referring to the same concept:a file path or an IO object.
This change aligns all class methods to use
path_or_io. This more accurately conveys that the argument accepts either a file path or an IO object.Reference